Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(causalConsistency): adding an example test for causal consistency #1651

Merged
merged 1 commit into from
Jan 30, 2018

Conversation

daprahamian
Copy link
Contributor

Fixes NODE-1262

NOTE: the version filter means the test isn't currently being run :(

@daprahamian daprahamian requested review from mbroadst and jlord January 22, 2018 20:57
Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

examples should all live in the examples_tests.js file

@daprahamian daprahamian force-pushed the NODE-1262 branch 2 times, most recently from 52cbc30 to d38294b Compare January 22, 2018 21:45
@daprahamian
Copy link
Contributor Author

This test is now in examples_tests.js, and should have no conflicts.

@@ -1,6 +1,7 @@
'use strict';

var assert = require('assert');
const expect = require('chai').expect;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the rest of the file be updated to use expect or should this test use the same format as the rest of the tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be a bit much for just this PR, would make it harder to read. Agreed that we should eventually do this in the future though.

Copy link
Contributor

@jlord jlord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a big deal so go ahead and merge as is, but per my in-line comment, it seems easier to just use the existing assert library rather than do this one test in a different way than the others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants